feat: migrate API client and codebase to native fetch (Part 1)#10723
feat: migrate API client and codebase to native fetch (Part 1)#10723joehan wants to merge 1 commit into
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the codebase from 'node-fetch' and 'abort-controller' to 'undici' for HTTP requests, updating proxy agent configurations, request signals, stream handling, and error mapping across multiple modules. Feedback on these changes highlights opportunities to improve type safety by avoiding 'any' in 'FetchOptions', adhering to the repository style guide by replacing 'console.error' with the central 'logger' in 'src/hosting/proxy.ts', and adding a defensive check in 'UndiciResponseCompat.text()' to prevent potential runtime errors when the response body is undefined.
| interface FetchOptions extends RequestInit { | ||
| dispatcher?: ProxyAgent; | ||
| duplex?: "half"; | ||
| agent?: any; | ||
| } |
There was a problem hiding this comment.
Avoid using any as an escape hatch for the agent property. Since noKeepAliveAgent is a function returning http.Agent | https.Agent, we can type this property more precisely to adhere to the repository style guide.
| interface FetchOptions extends RequestInit { | |
| dispatcher?: ProxyAgent; | |
| duplex?: "half"; | |
| agent?: any; | |
| } | |
| interface FetchOptions extends RequestInit { | |
| dispatcher?: ProxyAgent; | |
| duplex?: "half"; | |
| agent?: http.Agent | https.Agent | ((parsedURL: URL) => http.Agent | https.Agent); | |
| } |
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
| } catch (err: any) { | ||
| console.error("[PROXY ERROR]", err); | ||
| const isAbortError = |
There was a problem hiding this comment.
Use the central logger instead of console.error for logging errors to maintain consistency and adhere to the repository style guide.
| } catch (err: any) { | |
| console.error("[PROXY ERROR]", err); | |
| const isAbortError = | |
| } catch (err: any) { | |
| logger.error("[PROXY ERROR]", err); | |
| const isAbortError = |
References
- Use the central
logger(src/logger.ts); never useconsole.log()for user-facing output. (link)
| async text(): Promise<string> { | ||
| const { streamToString } = require("./utils"); | ||
| return streamToString(this.body); | ||
| } |
There was a problem hiding this comment.
Add a defensive check to ensure this.body is defined before passing it to streamToString. If this.body is null or undefined (e.g., for a 204 No Content response), calling streamToString(this.body) would throw a TypeError.
async text(): Promise<string> {
if (!this.body) {
return "";
}
const { streamToString } = require("./utils");
return streamToString(this.body);
}
joehan
force-pushed
the
refactor/migrate-to-native-fetch-part1
branch
from
4d2c3cd to
17625de
Compare
2 participants
Description
This is Part 1 of the native fetch migration. It contains the core codebase changes.
src/apiv2.ts) to use modern, nativeglobalThis.fetchin production.Set-Cookieheader forwarding, and location redirection fixes in the hosting proxy middleware.fetch.node-fetchandnockdevDependencies to maintain 100% backward compatibility for all existing unit tests under the test runner.This PR is the base for the stacked PRs: